Add UnwrappingReuseStrategy for AnalyzerWrapper #14154

mayya-sharipova · 2025-01-20T19:28:16Z

Add a new reuse strategy WrappingReuseStrategy that consults
he wrapped analyzer's strategy to decide if components can be
reused or need to be updated.

Add an optional constructor to CompletionAnalayzer that uses this strategy.

Add a new reuse strategy WrappingReuseStrategy that consults the wrapped analyzer's strategy to decide if components can be reused or need to be updated. Add an optional constructor to CompletionAnalayzer that uses this strategy.

john-wagster

LGTM

benwtrent

The API is a little concerning. I do not immediately know a better way to handle both wrappedComponents and also the inner storedValue :(

benwtrent · 2025-01-22T13:40:29Z

lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionAnalyzer.java

+  public CompletionAnalyzer(
+      Analyzer analyzer,
+      boolean preserveSep,
+      boolean preservePositionIncrements,
+      ReuseStrategy fallbackStrategy) {
+    super(new WrappingReuseStrategy(fallbackStrategy));
+    // häckidy-hick-hack, because we cannot call super() with a reference to "this":
+    ((WrappingReuseStrategy) getReuseStrategy()).setUp(this, analyzer, wrappedComponents);
+    this.analyzer = analyzer;
+    this.preserveSep = preserveSep;
+    this.preservePositionIncrements = preservePositionIncrements;
+    this.maxGraphExpansions = ConcatenateGraphFilter.DEFAULT_MAX_GRAPH_EXPANSIONS;
+  }


/** * JAVA DOCS about when you should use this and what it does */ public static CompletionAnalyzer withWrappingReuseStrategy( Analyzer analyzer, boolean preserveSep, boolean preservePositionIncrements, ReuseStrategy fallbackStrategy) { CompletionAnalyzer completionAnalyzer = new CompletionAnalyzer(analyzer, preserveSep, preservePositionIncrements, fallbackStrategy); ((WrappingReuseStrategy)completionAnalyzer.getReuseStrategy()).setUp(completionAnalyzer, analyzer, completionAnalyzer.wrappedComponents); return completionAnalyzer; }

What if we made the ctor private, removed the setup hack and did it instead within a public static method?

benwtrent · 2025-01-22T13:46:24Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+    public void setUp(
+        AnalyzerWrapper wrapper,
+        Analyzer wrappedAnalyzer,
+        CloseableThreadLocal<TokenStreamComponents> wrappedComponents) {
+      this.wrapper = wrapper;
+      this.wrappedAnalyzer = wrappedAnalyzer;
+      this.wrappedComponents = wrappedComponents;
+    }


This bugs me. We allow a reuse strategy to be mutated after construction. It seems ready to cause a bug.

benwtrent · 2025-01-22T13:47:22Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+
+    @Override
+    public TokenStreamComponents getReusableComponents(Analyzer analyzer, String fieldName) {
+      if (analyzer == wrapper) {


Do we really need to know the instance, or can we do Analyzer analyzer instanceof WrapperAnalyzer wrapperAnalyzer then get the strategy from wrapperAnalyzer.getWrappedAnalyzer(fieldName).getReuseStategy().getReusableComponents...?

benwtrent · 2025-01-22T13:47:44Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+    public void setReusableComponents(
+        Analyzer analyzer, String fieldName, TokenStreamComponents components) {
+      if (analyzer == wrapper) {
+        setStoredValue(analyzer, components);
+        wrappedAnalyzer
+            .getReuseStrategy()
+            .setReusableComponents(wrappedAnalyzer, fieldName, wrappedComponents.get());
+      } else {
+        fallbackStrategy.setReusableComponents(analyzer, fieldName, components);
+      }


Do we really need to know the instance, or can we do Analyzer analyzer instanceof WrapperAnalyzer wrapperAnalyzer then get the wrapped analyzer via getWrappedAnalyzer(fieldName)?

benwtrent · 2025-01-22T13:51:04Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+   * be re-created. During components creation, this analyzer must store the wrapped analyzer's
+   * components in {@code wrappedComponents} local thread variable.
+   */
+  public static final class WrappingReuseStrategy extends ReuseStrategy {


It seems to me that the better name is UnWrapping as if we find that the analyzer is a wrapped analyzer, we unwrap it and use the underlying analyzer?

benwtrent · 2025-01-22T13:58:04Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+    public void setUp(
+        AnalyzerWrapper wrapper,
+        Analyzer wrappedAnalyzer,
+        CloseableThreadLocal<TokenStreamComponents> wrappedComponents) {
+      this.wrapper = wrapper;
+      this.wrappedAnalyzer = wrappedAnalyzer;
+      this.wrappedComponents = wrappedComponents;
+    }


Ah, I see that the "wrappedComponents" is key here. its tricky, as the analyzer provides access directly to storedValue and there is no way to override :(.

That seems like a bad API already, but we don't want to change all these long lived interfaces unnecessarily.

jpountz · 2025-01-22T14:40:57Z

I don't like that CompletionAnalyzer needs to track a thread-local, the point of reuse strategy is to avoid this kind of thing. Also I'm not sure I understand why CompletionAnalyzer is different from other analyzer wrappers?

mayya-sharipova · 2025-01-23T14:51:42Z

@benwtrent Thanks for the review, I am not happy with the design either, will see how I can incorporate your feedback.

I don't like that CompletionAnalyzer needs to track a thread-local, the point of reuse strategy is to avoid this kind of thing. Also I'm not sure I understand why CompletionAnalyzer is different from other analyzer wrappers?

@jpountz Thanks for looking. The idea of having an extra thread-local is for AnalyzerWrapper to keep track of wrappedComponents, but may be it doesn't need to be thread-local and can be just a local variable.
You are right, CompletionAnalyzer is not different from other analyzer wrappers, and I can extend this policy to all AnalyzerWrapper. CompletionAnalyzer is where we experienced a problem in Elasticsearch, where it did not respect a reuse policy of the wrapped analyzer.

mayya-sharipova · 2025-01-23T16:51:38Z

@jpountz @benwtrent I've addressed your comments in the last commit, please continue to review

benwtrent

I am sorry @mayya-sharipova I am having a difficult time understanding the change and what its supposed to be doing.

Is the idea that we don't reuse wrapped components if the unwrapped component themselves cannot be reused?

lucene/analysis/common/src/java/org/apache/lucene/analysis/query/QueryAutoStopWordAnalyzer.java

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

…tegy

mayya-sharipova · 2025-01-31T17:04:42Z

I syncrhonized with @benwtrent offline, and we decided to use to extend TokenStreamComponents, so that it has access to wrapped components.

@benwtrent please review the last changes. Thank you.

benwtrent · 2025-01-31T17:30:23Z

I will have to review later, but first glance is way cleaner than before!

Thank you

benwtrent

I have a couple of comments, but its looking good.

benwtrent · 2025-02-03T19:23:26Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+      if (analyzer instanceof AnalyzerWrapper wrapper
+          && components instanceof TokenStreamComponentsWrapper wrapperComponents) {


could we add an assertion where if its passed an AnalyzerWrapper that the components are indeed instanceof TokenStreamComponentsWrapper?

benwtrent · 2025-02-03T19:24:16Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+   * A {@link Analyzer.TokenStreamComponents} that decorates the wrapper with access to the wrapped
+   * components.
+   */
+  public static final class TokenStreamComponentsWrapper extends TokenStreamComponents {


Does this need to be public? Could it maybe be package private?

benwtrent · 2025-02-03T19:27:15Z

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

+    @Override
+    protected void setReader(final Reader reader) {
+      wrapper.setReader(reader);
+    }


I wonder, do we need to override this? Isn't the default behavior simply setting the source which we already correctly provide via wrapper.getSource()?

benwtrent · 2025-02-03T19:27:44Z

lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java

@@ -387,7 +387,7 @@ public TokenStreamComponents(final Tokenizer tokenizer) {
     *
     * @param reader a reader to reset the source component
     */
-    private void setReader(final Reader reader) {
+    void setReader(final Reader reader) {


see comment about if we actually need to override this.

…tegy

benwtrent

Looks so much better! Thank you for iterating on the API. I like it.

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java

…r.java Co-authored-by: Benjamin Trent <[email protected]>

Add a new reuse strategy WrappingReuseStrategy that consults the wrapped analyzer's strategy to decide if components can be reused or need to be updated. Co-authored-by: Benjamin Trent <[email protected]>

jpountz · 2025-02-10T22:25:44Z

I don't feel good about this change (at least not yet). It looks like there is an analyzer that changes its components over time somewhere, and this change aims at making sure that an analyzer that wrap this analyzer would also update its components. I worry that this is just one place where the assumption that analysis components are immutable is violated, and that there are other ones that haven't been found yet. I'm curious if other alternatives were considered, e.g. atomically swapping the analyzer or updating the behavior of analysis components without needing to replace them.

For reference, a side-effect of this change is that analysis components are now cached both on the wrapper and on the wrapped analyzer, while they were only cached on the wrapper before.

mayya-sharipova · 2025-02-11T16:33:51Z

@jpountz Thank you for your feedback. I am happy to discuss alternatives.

Isn't the whole idea of ReuseStrategy to decide if an analyzer's components can be reused or should be recreated/swapped?

I worry that this is just one place where the assumption that analysis components are immutable is violated, and that there are other ones that haven't been found yet. I'm curious if other alternatives were considered, e.g. atomically swapping the analyzer or updating the behavior of analysis components without needing to replace them.

Yes, indeed in this PR analysis components are immutable, they are recreated/ swapped in an analyzer

For the context: this PR came from a need to have an updateable synonyms analyzer when it is wrapped in another analyzer, for example CompletionAnalyzer. In Elasticsearch an analyzer with updateable synonyms has a custom ReuseStrategy that creates a new components when synonyms are updated. We now want CompletionAnalyzer also update its components when its wrapped synonyms analyzer updated synonyms.

jpountz · 2025-02-11T21:14:20Z

I suspect that ReuseStrategy hasn't been built with the idea that it could be use to invalidate components, but only as a way to describe an efficient caching strategy for analysis components, typically whether analysis components can be cached globally (GLOBAL_REUSE_STRATEGY) or need a different cache entry on each field (PER_FIELD_REUSE_STRATEGY).

@uschindler I believe that you are the one who designed this logic, I'd be interested in your thoughts.

Add WrappingReuseStrategy for AnalyzerWrapper

5367209

Add a new reuse strategy WrappingReuseStrategy that consults the wrapped analyzer's strategy to decide if components can be reused or need to be updated. Add an optional constructor to CompletionAnalayzer that uses this strategy.

mayya-sharipova mentioned this pull request Jan 20, 2025

suggest searches against completion fields with a synonym set result in null pointer exceptions elastic/elasticsearch#114651

Open

john-wagster approved these changes Jan 21, 2025

View reviewed changes

benwtrent reviewed Jan 22, 2025

View reviewed changes

mayya-sharipova changed the title ~~Add WrappingReuseStrategy for AnalyzerWrapper~~ Add UnwrappingReuseStrategy for AnalyzerWrapper Jan 23, 2025

Address feedback

1045f73

benwtrent reviewed Jan 24, 2025

View reviewed changes

lucene/analysis/common/src/java/org/apache/lucene/analysis/query/QueryAutoStopWordAnalyzer.java Outdated Show resolved Hide resolved

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java Outdated Show resolved Hide resolved

mayya-sharipova added 2 commits January 31, 2025 11:43

Address feedback

497d54e

Merge remote-tracking branch 'upstream/main' into wrapping-reuse-stra…

a0564c7

…tegy

benwtrent reviewed Feb 3, 2025

View reviewed changes

benwtrent added this to the 10.2.0 milestone Feb 3, 2025

mayya-sharipova added 2 commits February 5, 2025 09:32

Address feedback

1f02dda

Merge remote-tracking branch 'upstream/main' into wrapping-reuse-stra…

d98432c

…tegy

github-actions bot added the module:core/search label Feb 5, 2025

Fix merge by mistake

2d98e3a

mayya-sharipova force-pushed the wrapping-reuse-strategy branch from 29e4a6f to 2d98e3a Compare February 5, 2025 14:45

github-actions bot added module:core/other and removed module:core/search labels Feb 5, 2025

benwtrent approved these changes Feb 5, 2025

View reviewed changes

lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java Show resolved Hide resolved

john-wagster approved these changes Feb 5, 2025

View reviewed changes

Update lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrappe…

732a507

…r.java Co-authored-by: Benjamin Trent <[email protected]>

mayya-sharipova merged commit ce2a917 into apache:main Feb 5, 2025
6 checks passed

mayya-sharipova deleted the wrapping-reuse-strategy branch February 5, 2025 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UnwrappingReuseStrategy for AnalyzerWrapper #14154

Add UnwrappingReuseStrategy for AnalyzerWrapper #14154

mayya-sharipova commented Jan 20, 2025

john-wagster left a comment

benwtrent left a comment

benwtrent Jan 22, 2025

benwtrent Jan 22, 2025

benwtrent Jan 22, 2025

benwtrent Jan 22, 2025

benwtrent Jan 22, 2025

benwtrent Jan 22, 2025

jpountz commented Jan 22, 2025

mayya-sharipova commented Jan 23, 2025

mayya-sharipova commented Jan 23, 2025

benwtrent left a comment

mayya-sharipova commented Jan 31, 2025

benwtrent commented Jan 31, 2025

benwtrent left a comment

benwtrent Feb 3, 2025

benwtrent Feb 3, 2025

benwtrent Feb 3, 2025

benwtrent Feb 3, 2025

benwtrent left a comment

jpountz commented Feb 10, 2025

mayya-sharipova commented Feb 11, 2025

jpountz commented Feb 11, 2025

		if (analyzer instanceof AnalyzerWrapper wrapper
		&& components instanceof TokenStreamComponentsWrapper wrapperComponents) {

Add UnwrappingReuseStrategy for AnalyzerWrapper #14154

Add UnwrappingReuseStrategy for AnalyzerWrapper #14154

Conversation

mayya-sharipova commented Jan 20, 2025

john-wagster left a comment

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented Jan 22, 2025

mayya-sharipova commented Jan 23, 2025

mayya-sharipova commented Jan 23, 2025

benwtrent left a comment

Choose a reason for hiding this comment

mayya-sharipova commented Jan 31, 2025

benwtrent commented Jan 31, 2025

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

jpountz commented Feb 10, 2025

mayya-sharipova commented Feb 11, 2025

jpountz commented Feb 11, 2025